Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Port SegmentationImage to the new image archetype style #6928

Merged
merged 7 commits into from
Jul 18, 2024
Merged

Port SegmentationImage to the new image archetype style #6928

merged 7 commits into from
Jul 18, 2024

Conversation

emilk
Copy link
Member

@emilk emilk commented Jul 17, 2024 •
edited by github-actions bot
Loading

What

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

@emilk emilk added 🚜 refactor Change the code, not the functionality include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages labels Jul 17, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 17, 2024 •
edited
Loading

Deployed docs

Commit Link
0e91315 https://landing-5zpji93d0-rerun.vercel.app/docs

@emilk emilk marked this pull request as draft July 17, 2024 15:34
@Wumpf Wumpf self-requested a review July 17, 2024 15:58
@emilk emilk marked this pull request as ready for review July 17, 2024 16:00
Wumpf
Wumpf approved these changes Jul 17, 2024
View reviewed changes
crates/store/re_types/src/archetypes/segmentation_image_ext.rs
return Err(ImageConstructionError::BadImageShape(tensor_data.shape));
}

let (blob, data_type) = match tensor_data.buffer {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe now is the time to add a conversion function that is shared between segmentation_image_ext and depth_image_ext?

rerun_cpp/src/rerun/archetypes/segmentation_image_ext.cpp

/// New segmentation image from tensor data.
/// Row-major.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are pretty sporadic, would be nice to write something nicer imho. Same on DepthImage, but didn't pay enough attention last time.

  • what is row major?
  • what does "borrows" mean? That's not a common concept in C++ and the implication (crashes at random) needs to be clarified

Copy link
Member Author

@emilk emilk Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a TODO to #6844

rerun_cpp/src/rerun/archetypes/segmentation_image_ext.cpp Outdated Show resolved Hide resolved
@emilk emilk mentioned this pull request Jul 18, 2024
Open
1 task
@emilk emilk merged commit a8bf4dd into main Jul 18, 2024
40 checks passed
@emilk emilk deleted the emilk/segmentation-image branch July 18, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wumpf Wumpf Wumpf approved these changes

Assignees
No one assigned
Labels
include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages 🚜 refactor Change the code, not the functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emilk @Wumpf